Skip to content

Conversation

@wolfv
Copy link
Contributor

@wolfv wolfv commented May 6, 2025

I started to work on this a little bit to make our parsing more robust against empty lines (e.g. when whitespace is removed) by counting the lines from the diff.

@wolfv
Copy link
Contributor Author

wolfv commented May 6, 2025

I think this is also needed to fix #2 because right now a line like -- will be counted as a "line to be removed" while it's actually the signature marker.

@whitty whitty mentioned this pull request Sep 30, 2025
@whitty
Copy link
Collaborator

whitty commented Oct 26, 2025

What is the state of this changed? is it intended to be complete?

FYI: test failures on regression patches:

This is failing on the chunk header - but see below.

https://github.com/gitpatch-rs/gitpatch/actions/runs/18760185548/job/53522404788?pr=3#step:5:146

failed to parse "tests/samples/sample1.diff", error: Line 13: Error while parsing:  @@ -5,16 +11,13 @@
...

This patch seems to be being mis-parsed possibly around the end-of-chunk detection?

https://github.com/gitpatch-rs/gitpatch/actions/runs/18760185548/job/53522404788?pr=3#step:5:192

failed to parse "tests/wild-samples/sample2.patch", error: Line 27: Error while parsing: diff --git a/js/src/jsfriendapi.cpp b/js/src/jsfriendapi.cpp
....

@sourcefrog
Copy link
Collaborator

It seems like conceptually a good idea to make use of the counts in the headers, but I'm not really sure from the PR or a brief read of the diff which one is meant to have priority if the number of lines expected disagrees with the lines that are apparently present?

@wolfv
Copy link
Contributor Author

wolfv commented Dec 8, 2025

Sorry for dropping the ball here. We've switched to our own fork of diffy and made it handle all edge cases we found in the wild: https://github.com/prefix-dev/diffy

IIRC there were some test cases here that did not contain valid diffs which made the line counting fail :) But I might misremember!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants